Support running with no timeout, ignore/deprecate TIMEOUT task param #2229
Conversation
When running in a Tekton task it's better if Tekton handles the timeout, so that's why we want to be able to do this. I decide to skip the context.WithTimeout entirely rather than just set it to 100 years or whatever. Ref: https://issues.redhat.com/browse/EC-1030
simonbaird
force-pushed
the
support-no-timeout
branch
from
277cfc5 to
e374d06
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2229 +/- ##
==========================================
+ Coverage 72.42% 72.69% +0.27%
==========================================
Files 88 88
Lines 7539 7622 +83
==========================================
+ Hits 5460 5541 +81
- Misses 2079 2081 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
It's a little more user friendly this way.
simonbaird
force-pushed
the
support-no-timeout
branch
from
e374d06 to
03d8150
Compare
| @@ -224,7 +226,8 @@ spec: | |||
| - "$(params.WORKERS)" | |||
| # NOTE: The syntax below is required to negate boolean parameters | |||
| - "--info=$(params.INFO)" | |||
| - "--timeout=$(params.TIMEOUT)" | |||
| # No timeout for EC, though Tekton may apply a timeout | |||
| - "--timeout=0" | |||
There was a problem hiding this comment.
This works quite badly on any version of EC that doesn't have this PR merged. It's probably fine (???), but I was considering using --timeout=100h for a while just to be extra cautious/paranoid.
There was a problem hiding this comment.
We can't release this version of the task without the new version of EC
There was a problem hiding this comment.
Agreed. I was wondering about the roll-out process. See this slack thread where @lcarva shared a doc that describes the details.
There was a problem hiding this comment.
Decided to be cautious. See new top commit.
There was a problem hiding this comment.
The official Task bundle is always built to use the latest EC CLI at the time (via OCI tag to a specific git commit ID). Also, updates to Konflux are always driven from Task updates. In other words, new CLI images are used indirectly via a new Task bundle. However, I agree with the general instance of being cautious with the change.
Ignore the TIMEOUT param. Mention in the param description that it's now ignored and deprecated. Ref: https://issues.redhat.com/browse/EC-1030
The commentary explains the reasoning. It's not expected to be an issue for Konflux, but theoretically RHTAP users might have an older EC base image and could some day migrate to this task definition which would break the task. Decided to keep the commit separate rather than squash so we can easily revert it some day. Ref: https://issues.redhat.com/browse/EC-1030
simonbaird
force-pushed
the
support-no-timeout
branch
from
03d8150 to
73f183e
Compare
See commits for details.
Ref: https://issues.redhat.com/browse/EC-1030